Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add name, description and non null tables to RLS #20432

Merged
merged 5 commits into from
Jun 20, 2022
Merged

feat: add name, description and non null tables to RLS #20432

merged 5 commits into from
Jun 20, 2022

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Jun 19, 2022 •
edited
Loading

SUMMARY

Add the following changes and improvements to Row Level Security:

  • Adds a unique name field (Adds a short name to each RLS, adds context for each RLS)
  • Adds a optional description field (adds the possibility to add a detailed description for each RLS)
  • Adds a validation that enforces that a new RLS must have an associated dataset (table)

List view

Screenshot 2022-06-19 at 19 16 12

Add view, when adding a new RLS with an already existing name

Screenshot 2022-06-19 at 19 16 57

Add view, validate at least one dataset is associated

Screenshot 2022-06-19 at 19 17 46

cc @altef

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dpgaspar dpgaspar requested a review from a team as a code owner June 19, 2022 17:21
@codecov
Copy link

codecov bot commented Jun 19, 2022 •
edited
Loading

Codecov Report

Merging #20432 (fdf5e69) into master (c2f01a6) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #20432      +/-   ##
==========================================
- Coverage   66.70%   66.57%   -0.13%     
==========================================
  Files        1739     1739              
  Lines       65153    65166      +13     
  Branches     6899     6899              
==========================================
- Hits        43462    43387      -75     
- Misses      19938    20026      +88     
  Partials     1753     1753
Flag Coverage Δ
hive 53.71% <100.00%> (+0.02%) ⬆️
mysql ?
postgres ?
presto 53.57% <100.00%> (+0.02%) ⬆️
python 82.49% <100.00%> (-0.28%) ⬇️
sqlite 82.12% <100.00%> (+<0.01%) ⬆️
unit 50.56% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/connectors/sqla/models.py 89.99% <100.00%> (-0.35%) ⬇️
superset/connectors/sqla/views.py 91.26% <100.00%> (+0.21%) ⬆️
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/views/database/mixins.py 60.34% <0.00%> (-20.69%) ⬇️
superset/sql_validators/__init__.py 80.00% <0.00%> (-20.00%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/common/utils/dataframe_utils.py 85.71% <0.00%> (-7.15%) ⬇️
superset/databases/commands/create.py 86.27% <0.00%> (-5.89%) ⬇️
superset/db_engine_specs/postgres.py 91.52% <0.00%> (-5.09%) ⬇️
superset/databases/api.py 90.67% <0.00%> (-4.83%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2f01a6...fdf5e69. Read the comment docs.

betodealmeida
betodealmeida approved these changes Jun 19, 2022
View reviewed changes
superset/migrations/versions/2022-06-19_16-17_f3afaf1f11f0_add_unique_name_desc_rls.py Outdated
# Set initial default names make sure we can have unique non null values
all_rls = session.query(RowLevelSecurityFilter).all()
for rls in all_rls:
rls.name = f"rls{rls.id}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

Suggested change
rls.name = f"rls{rls.id}"
rls.name = f"rls_{rls.id}"

@dpgaspar dpgaspar merged commit 60eb109 into apache:master Jun 20, 2022
@dpgaspar dpgaspar deleted the danielgaspar/sc-50677/superset-create-unique-name-and-disallow branch June 20, 2022 12:52
@dpgaspar dpgaspar mentioned this pull request Jun 21, 2022
Merged
9 tasks
akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022
@dpgaspar @akshatsri
feat: add name, description and non null tables to RLS (apache#20432)
4dd2082 
* feat: add name, description and non null tables to RLS

* add validation

* add and fix tests

* fix sqlite migration

* improve default value for name
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024
@dosubot dosubot bot mentioned this pull request May 22, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betodealmeida betodealmeida betodealmeida approved these changes

@villebro villebro Awaiting requested review from villebro

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L 🚢 2.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dpgaspar @betodealmeida @mistercrunch